-
-
Notifications
You must be signed in to change notification settings - Fork 100
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[nextest-filtering] Replace nom
parser with winnow
#1214
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #1214 +/- ##
==========================================
+ Coverage 77.40% 77.49% +0.09%
==========================================
Files 72 72
Lines 17859 17884 +25
==========================================
+ Hits 13823 13859 +36
+ Misses 4036 4025 -11 ☔ View full report in Codecov by Sentry. |
The explicit `complete` version is deprecated, so switching straight to `impl Parser for &str`, a shortcut for the auto-`complete` version.
`char` was consolidated into `one_of` but this jumped straight to the `impl Parser for char` which is easier to read.
`pair` is deprecated in favor of `impl Parser for (...)`
In winnow 0.5, we can replace this with `tag` and speed it up
Free-standing `recognize` is deprecated in favor of trait methods but it wasn't even needed here.
This added back in a `tag` that a later release (0.4?) can remove due to better type inference.
This added a lot of `tag` / `one_of`s that can be removed with a later release (0.4?)
This was possible due to improvements in type inference (when you have a generics problem, throw more `PhantomData` at it...).
In 0.5, `parse_next` modifies input so the old behavior is under the name `parse_peek` (as it doesn't commit to the parse but returns what it would commit). So `unpeek` takes a combinator that doesn't modify input ("peek parses") and makes it commit to the result modifying the input. This is added only to ease the transition. Future commits will work to remove `unpeek` calls.
Unsure why it was originally needed. One difference with `nom` is that we return `Stream::Slice` from `token` parsers, rather than `Stream`, simplifying the more common cases.
Because parsers now take `&mut I` and all cloning is gone, we don't need `RefCell` anymore.
Wow, thanks! Will look very soon. (you want to run cargo xfmt to fix the lint issue) |
This is awesome! Thank you. I'll go ahead and land this. |
As you use it, I welcome any feedback to help make it better! |
@sunshowers expressed interest in
winnow
and I figured porting an existing project they were familiar with would make it easier when they try it out.When i first switched from
nom
s pureFn(I) -> (I, O)
parser model toFn(&mut I) -> O
, I was concerned about it making things worse. It does have the downside of requiring more lifetime annotations to disambiguate things but I feel like it has been proven time and again to simplify code. In this case, it removed a lot of bookkeeping from the functions, making the intentional bookkeeping more obvious, and it allowed the removal ofRefCell
for error recovery.I broke this up into a lot of commits
I did put a little too much in the
unpeek
-ectomy commit towards the end. The main downside is that we can'tgit bisect
to one specific bookkeeping change.I did this as 1 PR though because the decision to commit to this is all-or-nothing.